Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support string and array multiline, and integer representation encoding #183

Merged
merged 13 commits into from
Feb 22, 2023
Merged

Support string and array multiline, and integer representation encoding #183

merged 13 commits into from
Feb 22, 2023

Conversation

NightEule5
Copy link
Collaborator

Fixes #161

@NightEule5 NightEule5 requested a review from orchestr7 January 28, 2023 09:35
@BOOMeranGG
Copy link
Collaborator

BOOMeranGG commented Jan 29, 2023 •
edited
Loading

One more suggestion is:
Do the same with grouping as in https://kotlinlang.org/docs/numbers.html#literal-constants-for-numbers (to be able to use GROUPED along with BINARY/OCTAL/HEX)

To get the following result:

hexBytes = 0xFF_EC_DE_5E
bytes = 0b11010010_01101001_10010100_10010010

What do you think?

@NightEule5
Copy link
Collaborator Author

Yep, I thought about that and it's definitely doable, but I figured that would be a feature to add later. We can make an issue about this.

There's two ways I could see this being represented in the TomlInteger annotation. We could simply remove the GROUPED enum value and use a groupSize property instead, 0 being no grouping. We could make a separate enum, like IntegerGrouping, with common groupings like THOUSANDS or BYTES.

I think the first would be preferred

@BOOMeranGG
Copy link
Collaborator

We could simply remove the GROUPED enum value and use a groupSize property instead, 0 being no grouping.

If groupSize = 3 what does it mean? Will it be like this: 1_000_000 ?
Also for the first approach we could add constants for common groupings

@NightEule5
Copy link
Collaborator Author

If groupSize = 3 what does it mean? Will it be like this: 1_000_000 ? Also for the first approach we could add constants for common groupings

Yep, that's the idea

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 31, 2023
View reviewed changes
ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/writers/TomlEmitter.kt Outdated
)
}

//private fun emitInt(value: Long)

Check failure

Code scanning / ktlint

[COMMENT_WHITE_SPACE] there should be a white space between code and comment also between code start token and comment text: There should be 1 space(s) before comment token in //private fun emitInt(value: Long)

[COMMENT_WHITE_SPACE] there should be a white space between code and comment also between code start token and comment text: There should be 1 space(s) before comment token in //private fun emitInt(value: Long)
@NightEule5
Add groupSize parameter and change grouping to chunked string
d483f17
@NightEule5
Merge branch 'main' into int-repr-multiline
5c367a2 
# Conflicts:
#	ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/tree/nodes/pairs/values/TomlBasicString.kt
@NightEule5
Clean up after merge
fc76863
@NightEule5
Copy link
Collaborator Author

Surprisingly not much conflicts

@orchestr7
Copy link
Owner

Surprisingly not much conflicts

yeah, I am also confused - came here to look at conflicts :)
Will review and merge it today

@orchestr7
Copy link
Owner

It will also be good to add a description with all such annotations like @TomlMultiline to readme. But can be done in a separate issue. Otherwise, I'm afraid, we will get a lot of questions about that

orchestr7
orchestr7 reviewed Feb 21, 2023
View reviewed changes
ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/tree/nodes/pairs/values/TomlLong.kt
) : TomlValue() {
public constructor(content: String, lineNo: Int) : this(content.toLong())
public constructor(content: String, lineNo: Int) : this(content.parse())
Copy link
Owner

@orchestr7 orchestr7 Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I do not get it - if you remember I was always avoiding custom logic for parsing and even made logic for parsing based on exceptions:

try {
    s.toLong
} catch() {
    try {
        s.toInt
    }
}

...
etc.

So I tried as much as possible to avoid custom parsing and use parsing from stdlib. Why do we need to have it here?

Copy link
Collaborator Author

@NightEule5 NightEule5 Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I did it this way to support parsing the other integer types, instead of just decimal. toLong only parses decimal numbers, it can't handle the prefixes and non-decimal bases that TOML supports, like 0b, 0x, etc. We can do it a different way if you have something in mind

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that this parsing of prefixes is included into toLong… I got the intention now

orchestr7
orchestr7 reviewed Feb 21, 2023
View reviewed changes
...core/src/commonMain/kotlin/com/akuleshov7/ktoml/tree/nodes/pairs/values/TomlLiteralString.kt
when {
multiline ->
when {
multilineControlCharacterRegex in this ->
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regular expressions affect performance and in some cases can lead to exceptions in case of very long strings...
May it will be better to use something like string.any(setOfillegalCharacters::contains) instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, admittedly regexes aren't needed here. I assume you want a separate issue for this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I will make a separate one

orchestr7
orchestr7 approved these changes Feb 21, 2023
View reviewed changes
Copy link
Owner

@orchestr7 orchestr7 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically lgtm, but I am a little bit worried about:

  1. custom parsing of Long
  2. a little bit unreadable regular expressions that can also affect the performance

@orchestr7 orchestr7 merged commit 94c0b3d into orchestr7:main Feb 22, 2023
@orchestr7 orchestr7 mentioned this pull request Feb 22, 2023
Closed
@NightEule5 NightEule5 deleted the int-repr-multiline branch February 23, 2023 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orchestr7 orchestr7 orchestr7 approved these changes

@BOOMeranGG BOOMeranGG BOOMeranGG approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support TomlMultiline and TomlInteger annotations
3 participants
@NightEule5 @BOOMeranGG @orchestr7